Skip to content

Add waitForAllTaskTermination#21

Closed
norio-nomura wants to merge 1 commit intoCarthage:upgrade-racfrom
norio-nomura:wait-for-all-task-termination
Closed

Add waitForAllTaskTermination#21
norio-nomura wants to merge 1 commit intoCarthage:upgrade-racfrom
norio-nomura:wait-for-all-task-termination

Conversation

@norio-nomura
Copy link
Copy Markdown
Contributor

@norio-nomura norio-nomura commented May 27, 2015

Fix segmentation fault on Carthage termination.
Carthage/Carthage#474 (comment)

@norio-nomura
Add waitForAllTaskTermination
f35b15f 
Fix segmentation fault on Carthage termination.
Carthage/Carthage#474 (comment)
@norio-nomura norio-nomura mentioned this pull request May 28, 2015
Closed
@jspahrsummers jspahrsummers self-assigned this May 28, 2015
jspahrsummers
jspahrsummers reviewed May 28, 2015
View reviewed changes
Comment thread ReactiveTask/Task.swift
Copy link
Copy Markdown
Member

@jspahrsummers jspahrsummers May 28, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be created within launchTask, since TaskDescriptions can be reused across multiple task invocations.

@jspahrsummers
Copy link
Copy Markdown
Member

jspahrsummers commented May 28, 2015

Thanks for the pull request! ✨

I think I've misunderstood the purpose, so my comments above may not be valuable. If we need to wait for all tasks to complete before exiting on the main thread, I'd prefer to incorporate that into our SignalProducers somehow (instead of calling a special function before termination—which is easy to forget).

@norio-nomura
Copy link
Copy Markdown
Contributor Author

norio-nomura commented May 28, 2015

I did want to prove what happened and how can be fixed. 😄
I couldn't fix it with RAC way by using SignalProducer, so I wrote this instead.
Yes, it should be fixed on the way you prefer. 👍

@jspahrsummers
Copy link
Copy Markdown
Member

jspahrsummers commented May 28, 2015

Ah, okay. I'm happy to give it a shot!

Thank you so much for finding the issue. 🙇⭐

@norio-nomura
Copy link
Copy Markdown
Contributor Author

norio-nomura commented May 28, 2015

I hope that all reported segmentation fault on termination are on this scenario and will be fixed. 🙏

@jspahrsummers jspahrsummers mentioned this pull request May 28, 2015
Merged
@jspahrsummers
Copy link
Copy Markdown
Member

jspahrsummers commented May 28, 2015

Should be fixed by #23. 🎉

Thanks again for finding that!

@norio-nomura norio-nomura deleted the wait-for-all-task-termination branch May 28, 2015 07:59
@norio-nomura
Copy link
Copy Markdown
Contributor Author

norio-nomura commented May 28, 2015

I'm sorry I couldn't write proper code for fix this by my self.

@robrix
Copy link
Copy Markdown

robrix commented May 28, 2015

@norio-nomura No apologies necessary, you contributed a great deal by diagnosing the issue 🙇

@norio-nomura
Copy link
Copy Markdown
Contributor Author

norio-nomura commented May 28, 2015

@robrix Thanks 😃

@norio-nomura norio-nomura restored the wait-for-all-task-termination branch June 2, 2015 12:50
This was referenced Jun 2, 2015
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@jspahrsummers jspahrsummers

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@norio-nomura @jspahrsummers @robrix